Skip to content

Conversation

@cgyurgyik
Copy link
Contributor

@cgyurgyik cgyurgyik commented Aug 7, 2020

Description

  • Adds unnamed namespaces around helper functions. This ensures that they don't escape the scope of the file. This is the "modern way" to declare them as static.
  • To improve readability, adds functions with documentation (and some examples) to the SphericalVoxelGrid container. This allows for const correctness for member variables as well, as their values should not change during the lifetime of the container.
  • Removes optimization that reduces number of calculations when num_polar_voxels == num_azimuthal_voxels. This can be added again at a later time if it proves to be a common case and efficient. For now, it just adds clutter. Same thing for initializeVoxelBoundarySegments.
  • Only checks inBoundsAngular and inBoundsAzimuthal when an angular/voxel/both are hit. @ak-2485 please verify I'm using these correctly. For example, you set it to only check inboundsAzimuthal even when it was both a Polar and an Azimuthal hit. So I may be missing something here.
  • Some documentation cleanup.

How Has This Been Tested?

None required. Semantics should remain the same.

Checklist:

  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • New and existing unit tests pass locally with my changes

@ak-2485
Copy link
Contributor

ak-2485 commented Aug 11, 2020

This looks good. I think that I only checked one of the inbounds functions in cases when the polar and azimuthal hits are simultaneous, but I see that this is potentially incorrect.

@matthewturk
Copy link
Contributor

matthewturk commented Aug 11, 2020 via email

@cgyurgyik cgyurgyik merged commit 5b70b61 into spherical-volume-rendering:master Aug 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants